Skip to content

fix(tables): enforce plan limits in mothership user_table tool#4832

Merged
TheodoreSpeaks merged 3 commits into
stagingfrom
feat/bump-table-row-limit
Jun 2, 2026
Merged

fix(tables): enforce plan limits in mothership user_table tool#4832
TheodoreSpeaks merged 3 commits into
stagingfrom
feat/bump-table-row-limit

Conversation

@TheodoreSpeaks
Copy link
Copy Markdown
Collaborator

Summary

  • Mothership user_table tool (create, create_from_file) now resolves the workspace plan limits and passes maxRows/maxTables to createTable, matching the HTTP routes — previously it fell back to hardcoded defaults (10k rows / 100 tables) and bypassed plan caps
  • create_from_file now rejects files exceeding the plan row limit up front (clean message, no orphan table) and deletes the table if row insertion fails mid-import

Type of Change

  • Bug fix

Testing

Tested manually; added unit tests (4 new, 17 passing). bun run lint and bun run check:api-validation:strict pass.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link
Copy Markdown

vercel Bot commented Jun 1, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jun 1, 2026 7:53pm

Request Review

@TheodoreSpeaks
Copy link
Copy Markdown
Collaborator Author

@greptile review

@cursor
Copy link
Copy Markdown

cursor Bot commented Jun 1, 2026

PR Summary

Medium Risk
Changes billing-enforced table creation and file-import behavior in the copilot path; rollback on failure reduces orphan tables but failed cleanup could still leave edge cases.

Overview
The Mothership user_table tool now loads workspace plan limits via getWorkspaceTableLimits and passes maxRows / maxTables into createTable for create and create_from_file, aligning with the HTTP table routes instead of implicit defaults.

For create_from_file, imports are capped to the plan’s per-table row limit (extra rows are skipped and called out in the success message). If row insertion fails after the table is created, the tool deletes the new table and returns a rollback message. Unit tests cover limit stamping, truncation messaging, and rollback on insert failure.

Reviewed by Cursor Bugbot for commit 52dd4ca. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 1, 2026

Greptile Summary

This PR fixes plan-limit enforcement in the mothership user_table copilot tool so that create and create_from_file now resolve the workspace billing plan via getWorkspaceTableLimits and pass maxRows/maxTables to createTable, matching the existing HTTP routes. Previously these operations fell back to hardcoded defaults and bypassed per-plan caps.

  • create: one-line fix — fetches plan limits before createTable and passes them through.
  • create_from_file: fetches plan limits, truncates the row set to the plan cap (reporting dropped rows), and rolls back the newly-created table with a clean error message when batch insertion fails.
  • Tests: 4 new tests verify limit stamping, row truncation, and rollback behaviour.

Confidence Score: 4/5

Safe to merge after reviewing the truncation guard — all other changes are correct and well-tested.

The row-truncation arithmetic in create_from_file does not handle non-positive maxRowsPerTable values. Because plan limits are read through envNumber env-var overrides and the existing getMaxRowsPerTable helper documents -1 as the unlimited sentinel, a self-hosted operator who sets e.g. FREE_TABLE_ROWS_LIMIT=-1 would silently have the last row of every CSV import dropped. The fix is a one-line guard before the slice. Everything else — limit stamping, rollback logic, error messages, and tests — is solid.

apps/sim/lib/copilot/tools/server/table/user-table.ts — specifically the two truncation lines at ~770-771.

Important Files Changed

Filename Overview
apps/sim/lib/copilot/tools/server/table/user-table.ts Adds plan-limit fetching to create and create_from_file; truncation arithmetic at line 770-771 does not guard against non-positive maxRowsPerTable values that env-var overrides could introduce.
apps/sim/lib/copilot/tools/server/table/user-table.test.ts Four new test suites cover plan-limit stamping, row truncation, and rollback. Mocks are correctly wired; no issues found.

Sequence Diagram

sequenceDiagram
    participant C as Copilot Tool
    participant B as getWorkspaceTableLimits
    participant S as createTable (service)
    participant I as batchInsertAll

    Note over C: create operation
    C->>B: getWorkspaceTableLimits(workspaceId)
    B-->>C: "{ maxRowsPerTable, maxTables }"
    C->>S: "createTable({ ..., maxRows, maxTables })"
    S-->>C: table

    Note over C: create_from_file operation
    C->>C: parseFileRows(file)
    C->>B: getWorkspaceTableLimits(workspaceId)
    B-->>C: "{ maxRowsPerTable, maxTables }"
    C->>C: truncate rows to maxRowsPerTable
    C->>S: "createTable({ ..., maxRows, maxTables })"
    S-->>C: table
    C->>I: batchInsertAll(rowsToImport)
    alt insert succeeds
        I-->>C: insertedCount
        C-->>C: return success (+ dropped-rows warning if any)
    else insert fails
        I-->>C: throw insertError
        C->>S: deleteTable(table.id, cleanupRequestId)
        C-->>C: return failure with rollback message
    end
Loading

Reviews (5): Last reviewed commit: "fix(tables): log rollback failures and s..." | Re-trigger Greptile

Comment thread apps/sim/lib/copilot/tools/server/table/user-table.ts
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 1, 2026

Greptile Summary

This PR wires the workspace billing plan into the mothership user_table tool so that create and create_from_file pass real maxRows/maxTables limits to createTable, matching how the HTTP routes already behave. It also adds an early file-size guard in create_from_file and cleans up any newly-created table when row insertion fails mid-import.

  • getWorkspaceTableLimits(workspaceId) is now called in both create and create_from_file before the table is created, and the returned maxRowsPerTable / maxTables are forwarded to createTable.
  • create_from_file rejects files that exceed the plan row limit before creating a table, and wraps batchInsertAll in a try/catch that deletes the just-created table on failure before re-throwing.
  • Four new unit tests cover limit stamping, pre-check rejection, and insert-failure cleanup, all passing with correct mock wiring.

Confidence Score: 3/5

Safe to merge after verifying how the service layer uses requestId — if it is purely for logging/tracing the cleanup path is fine; if it is an idempotency key the orphan-table risk is real.

The core logic — fetching plan limits and passing them to createTable — is correct and well-tested. The concern is in the cleanup path: deleteTable is called with the same requestId that was used for createTable, diverging from every other deleteTable callsite in the file. If the service layer treats requestId as an idempotency key, this would silently skip the cleanup delete and leave an orphan table after a failed import.

apps/sim/lib/copilot/tools/server/table/user-table.ts — specifically the cleanup deleteTable call inside the create_from_file catch block, and the lib/table/service implementation of deleteTable to confirm how requestId is consumed.

Important Files Changed

Filename Overview
apps/sim/lib/copilot/tools/server/table/user-table.ts Adds plan-limit enforcement to create and create_from_file: fetches billing limits, pre-validates row count, passes maxRows/maxTables to createTable, and attempts table cleanup on insert failure. The cleanup reuses the same requestId as the original createTable call, which may silently skip the delete if the service layer treats requestId as an idempotency key.
apps/sim/lib/copilot/tools/server/table/user-table.test.ts Adds 4 new tests covering plan-limit stamping, pre-check rejection, and insert-failure cleanup. Mock wiring is correct (mocks @/lib/table/billing and @/lib/table/service separately, matching the barrel re-exports). Tests cover the core new paths but do not assert the cleanup deleteTable call uses a fresh requestId.

Sequence Diagram

sequenceDiagram
    participant C as Caller
    participant T as userTableServerTool
    participant B as getWorkspaceTableLimits
    participant S as createTable (service)
    participant I as batchInsertAll
    participant D as deleteTable (service)

    C->>T: execute(create / create_from_file, ...)
    T->>B: getWorkspaceTableLimits(workspaceId)
    B-->>T: "{ maxRowsPerTable, maxTables }"

    alt create_from_file only
        T->>T: "rows.length > maxRowsPerTable?"
        T-->>C: error (no table created)
    end

    T->>S: "createTable({ maxRows, maxTables, ... }, requestId)"
    S-->>T: table

    alt create_from_file only
        T->>I: batchInsertAll(table.id, coerced, ...)
        alt insertion succeeds
            I-->>T: inserted count
            T-->>C: success
        else insertion throws
            I-->>T: error
            T->>D: deleteTable(table.id, requestId)
            D-->>T: cleanup errors swallowed
            T-->>C: error rethrown to outer catch
        end
    else create only
        T-->>C: success
    end
Loading

Reviews (2): Last reviewed commit: "fix(tables): enforce plan limits in moth..." | Re-trigger Greptile

Comment thread apps/sim/lib/copilot/tools/server/table/user-table.ts
Comment thread apps/sim/lib/copilot/tools/server/table/user-table.ts
@TheodoreSpeaks
Copy link
Copy Markdown
Collaborator Author

@greptile review

@TheodoreSpeaks
Copy link
Copy Markdown
Collaborator Author

@greptile review

@waleedlatif1
Copy link
Copy Markdown
Collaborator

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator

@cursor review

@TheodoreSpeaks TheodoreSpeaks merged commit fc7e35e into staging Jun 2, 2026
14 checks passed
@TheodoreSpeaks TheodoreSpeaks deleted the feat/bump-table-row-limit branch June 2, 2026 00:12
Comment on lines +770 to +771
const droppedRows = Math.max(0, rows.length - planLimits.maxRowsPerTable)
const rowsToImport = droppedRows > 0 ? rows.slice(0, planLimits.maxRowsPerTable) : rows
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 The truncation arithmetic does not guard against planLimits.maxRowsPerTable being 0 or negative. getTablePlanLimits reads its values through envNumber env-var overrides without a minimum-value check, and the private getMaxRowsPerTable helper explicitly documents -1 as the sentinel for "unlimited". If any override is set to 0, all rows are dropped silently and an empty table is created. If it is set to -1, rows.slice(0, -1) drops the last row from every import and produces the nonsensical message "exceeded plan limit of -1 rows".

Suggested change
const droppedRows = Math.max(0, rows.length - planLimits.maxRowsPerTable)
const rowsToImport = droppedRows > 0 ? rows.slice(0, planLimits.maxRowsPerTable) : rows
// Treat 0 or negative as "no limit" (env-var overrides may use -1 for unlimited).
const effectiveMaxRows =
planLimits.maxRowsPerTable > 0 ? planLimits.maxRowsPerTable : Infinity
const droppedRows = Math.max(0, rows.length - effectiveMaxRows)
const rowsToImport = droppedRows > 0 ? rows.slice(0, effectiveMaxRows) : rows

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 52dd4ca. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants